Skip to content

fix: extend worktree ownership guard to resolver adoption paths#1206

Merged
Wirasm merged 3 commits intodevfrom
fix/resolver-worktree-ownership-guard
Apr 14, 2026
Merged

fix: extend worktree ownership guard to resolver adoption paths#1206
Wirasm merged 3 commits intodevfrom
fix/resolver-worktree-ownership-guard

Conversation

@Wirasm
Copy link
Copy Markdown
Collaborator

@Wirasm Wirasm commented Apr 14, 2026

Summary

Closes the cross-clone gap that #1198 left open. That PR guarded `WorktreeProvider.findExisting()` (path 4 of 4), but `IsolationResolver` has three earlier adoption paths that bypass the provider layer entirely. Two clones of the same remote share `codebase_id` (identity is derived from `owner/repo`), so clone B could still silently adopt clone A's worktree via any of them.

Fixes #1183
Fixes #1188 (part 1 — cross-checkout)

Paths guarded

# Path Matched by Behavior on cross-clone
1 `findReusable` DB lookup on `codebase_id + workflowType + workflowId` Throw — DB row preserved
2 `findLinkedIssueEnv` DB lookup per linked issue Throw — DB row preserved
3 `tryBranchAdoption` git `findWorktreeByBranch` Throw — no DB row written
4 `WorktreeProvider.findExisting` filesystem `worktreeExists` Already guarded by #1198

DB rows are never destroyed on cross-clone — they legitimately belong to the other clone.

Changes

File Change
`packages/git/src/worktree.ts` Extract `verifyWorktreeOwnership` as an exported function, next to `getCanonicalRepoPath` which parses the same `.git` file format
`packages/git/src/index.ts` Re-export
`packages/isolation/src/providers/worktree.ts` Remove private method, import from `@archon/git`
`packages/isolation/src/resolver.ts` Compute `canonicalRepoPath` once at top of `resolve()`, pass it to all three adoption paths, call `verifyWorktreeOwnership` in each
`packages/isolation/src/resolver.test.ts` 6 new tests (cross-clone + same-clone happy path per guarded path)

Deferred

Explicitly not in this PR, to keep scope tight:

Behavior decision

On cross-checkout, all three paths throw consistently with `findExisting`. Rationale:

Testing

  • `bun run type-check` — clean
  • `bun run lint` — zero warnings
  • `bun test packages/isolation/src/resolver.test.ts` — 34 pass (6 new)
  • `bun test packages/isolation/src/providers/worktree.test.ts` — 126 pass (unchanged)
  • Full test suite — all green

Attribution

Thanks to @halindrome for the three-issue root-cause mapping (#1183, #1186, #1188, #1192). #1186's metadata-based DB-level guard framed exactly the resolver paths this PR targets with a stricter live-check approach.

Summary by CodeRabbit

  • Bug Fixes
    • Prevented workflows from adopting worktrees or DB state that belong to a different local clone (cross-clone isolation hardened); verification now fails loudly on mismatches.
  • Refactor
    • Consolidated worktree ownership verification into a shared utility used across all adoption paths.
  • Documentation
    • Added troubleshooting guidance explaining cross-clone ownership errors and remediation steps.
  • Tests
    • Added tests covering ownership verification, error cases, and adoption refusal scenarios.

#1188)

PR #1198 guarded WorktreeProvider.findExisting(), but IsolationResolver
has three earlier adoption paths that bypass the provider layer:

- findReusable (DB lookup by workflow identity)
- findLinkedIssueEnv (cross-reference via linked issues)
- tryBranchAdoption (PR branch discovery)

Two clones of the same remote share codebase_id (identity is derived
from owner/repo). Without these guards, clone B silently adopts clone
A's worktree via any of the three paths.

Changes:
- Extract verifyWorktreeOwnership from WorktreeProvider (private) to
  @archon/git/src/worktree.ts as an exported function, sitting next to
  getCanonicalRepoPath which parses the same .git file format
- Call the shared function from all three resolver paths; throw on
  cross-clone mismatch (DB rows are preserved — they legitimately
  belong to the other clone)
- Compute canonicalRepoPath once at the top of resolve()
- Six new tests in resolver.test.ts covering each guarded path's
  cross-checkout and same-clone behaviors

Fixes #1183
Fixes #1188 (part 1 — cross-checkout; part 2 parallel collision deferred
to follow-up alongside #1036)
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 14, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: ce9c6f93-8bd8-47cc-abcb-53c9dabb1780

📥 Commits

Reviewing files that changed from the base of the PR and between c8f2e0c and f047e91.

📒 Files selected for processing (1)
  • packages/git/src/worktree.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/git/src/worktree.ts

📝 Walkthrough

Walkthrough

Extracted a shared verifyWorktreeOwnership into the git package and wired it through isolation resolution/adoption paths so reusable/linked/adopted worktrees are validated against the requesting clone before being returned or adopted.

Changes

Cohort / File(s) Summary
Git package exports
packages/git/src/index.ts
Added public export for verifyWorktreeOwnership.
Worktree ownership verification
packages/git/src/worktree.ts
Added verifyWorktreeOwnership(worktreePath, expectedRepo) that reads .git, parses a gitdir: worktrees pointer, normalizes & compares resolved paths, and tightens FS error handling (preserves errno, rejects on EISDIR and other read failures).
Isolation provider
packages/isolation/src/providers/worktree.ts
Removed in-class verification, imported and calls shared verifyWorktreeOwnership; added try/catch to log worktree.adoption_refused_cross_checkout with context and rethrow on mismatch.
Isolation resolver
packages/isolation/src/resolver.ts
Threaded canonicalRepoPath into findReusable, findLinkedIssueEnv, tryBranchAdoption; added assertWorktreeOwnership wrapper calling shared verifier and logging/rethrowing on mismatch; removed duplicate canonical lookups.
Tests (resolver & provider)
packages/isolation/src/resolver.test.ts, packages/isolation/src/providers/worktree.test.ts
Added spies and tests for cross-clone rejection and same-clone acceptance paths; updated mocks to assert ownership checks block adoption when .git points to a different clone.
Git tests
packages/git/src/git.test.ts
Added unit tests for verifyWorktreeOwnership covering valid pointer, different-clone rejection, path normalization, EISDIR and ENOENT preservation, submodule and corrupted .git cases.
Docs & changelog
packages/docs-web/src/content/docs/reference/troubleshooting.md, CHANGELOG.md
Added troubleshooting entry explaining cross-clone worktree errors and remediation steps; added changelog note about cross-clone worktree isolation fix.

Sequence Diagram(s)

sequenceDiagram
  participant Resolver
  participant Provider
  participant GitPkg
  participant FS
  Resolver->>Provider: resolve/adopt worktree request (canonicalRepoPath)
  Provider->>GitPkg: verifyWorktreeOwnership(worktreePath, canonicalRepoPath)
  GitPkg->>FS: read <worktreePath>/.git
  FS-->>GitPkg: .git contents (gitdir: .../.git/worktrees/...)
  GitPkg->>GitPkg: resolve paths & compare normalized repo path
  alt match
    GitPkg-->>Provider: ok
    Provider-->>Resolver: proceed with adoption/return
  else mismatch or error
    GitPkg-->>Provider: throw error (belongs to different clone / EISDIR / ENOENT)
    Provider-->>Resolver: log warning and rethrow
  end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related issues

Possibly related PRs

Poem

🐰
I hopped through .git with a curious paw,
Traced gitdir: roots to see what I saw.
"Is this worktree mine?" I asked with a twitch—
Now verify guards every adopt-and-switch.
Hooray for tidy clones and safer kits!

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: extending worktree ownership verification to resolver adoption paths, which is the core focus of this PR.
Description check ✅ Passed The PR description includes Summary (with problem/scope), detailed Paths table, Changes table, Deferred work, Behavior decision, Testing evidence, and Attribution—covering most template sections comprehensively.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/resolver-worktree-ownership-guard

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@Wirasm
Copy link
Copy Markdown
Collaborator Author

Wirasm commented Apr 14, 2026

PR Review Summary (multi-agent)

Reviewed by: code-reviewer, docs-impact, pr-test-analyzer, silent-failure-hunter, code-simplifier.

Verdict: NEEDS MINOR FIXES

Core correctness is solid — canonicalRepoPath is computed once at the top of resolve() and gates all three adoption paths before any DB mutation; no silent swallowing anywhere; classifyIsolationError covers every error message pattern; throw-on-cross-clone is consistent with path 4 (#1198). Issues below are polish/observability and one debatable semantic.

Important Issues

# Agent Issue Location
1 code-reviewer findLinkedIssueEnv throws on first cross-clone issue — remaining linked issues are never evaluated. If issue N is another clone's worktree but issue N+1 is a valid same-clone worktree, the user gets a hard error instead of adoption. Either continue on ownership failure, or add a code comment explaining why whole-resolution failure is intentional. packages/isolation/src/resolver.ts:290-300
2 code-reviewer Log event worktree_adoption_refused_cross_checkout doesn't follow {domain}.{action}_{state} convention (CLAUDE.md). Should be worktree.adoption_refused_cross_checkout. packages/isolation/src/providers/worktree.ts:499
3 silent-failure-hunter Cross-clone rejection logs in findReusable and tryBranchAdoption omit codebaseId/canonicalRepoPath as structured fields — only present inside err.message string. Hurts future incident debugging. resolver.ts:241-251, 341-349
4 silent-failure-hunter verifyWorktreeOwnership strips err.code (EACCES, EIO, etc.) when re-wrapping filesystem errors. classifyIsolationError currently matches on substrings of the composed message, which works today but is fragile to Node.js message format changes. Use { cause: err } or assign .code on the wrapped error. packages/git/src/worktree.ts:301

Suggestions

Agent Suggestion Location
pr-test-analyzer No unit tests for verifyWorktreeOwnership itself — the actual .git file parsing (regex, EISDIR/ENOENT branching, resolve() normalization) is only exercised via resolver-level spies. Add cases to packages/git/src/: valid pointer match, repo mismatch, EISDIR (full checkout), ENOENT, non-matching file content, path normalization. Rating 7/10. packages/git/src/worktree.ts:259-325
pr-test-analyzer tryBranchAdoption cross-clone test asserts the throw but doesn't assert store.create was never called. Paths 1 and 2 assert updateStatus not called — path 3 should be symmetric. Rating 5/10. packages/isolation/src/resolver.test.ts:937
silent-failure-hunter getCanonicalRepoPath(codebase.defaultCwd) at top of resolve() has no wrapping log. On failure, it propagates as an unclassified crash rather than a known isolation failure. resolver.ts:111
silent-failure-hunter Pre-existing / out of scope — secondary findWorktreeByBranch path inside WorktreeProvider.findExisting (the isPRIsolationRequest branch) has no ownership verification. Same class of bug as this PR fixes. Worth a follow-up issue. providers/worktree.ts:509-521
code-simplifier The warn-then-rethrow pattern repeats identically three times. Extract a private assertWorktreeOwnership(worktreePath, canonicalRepoPath, logContext, logEvent) helper on IsolationResolver — saves ~20 lines and centralizes the contract. Rule of Three satisfied. resolver.ts:238-251, 287-300, 337-349
code-simplifier In findReusable/findLinkedIssueEnv, toWorktreePath(existing.working_path) is called twice. Extract a local const worktreePath = toWorktreePath(...) above the worktreeExists check. resolver.ts:237, 287

Documentation

File Issue
CHANGELOG.md No [Unreleased] entry for the three-PR cross-clone fix series (#1198, #1206, #1183, #1188). User-facing correctness fix — belongs in ### Fixed.
packages/docs-web/src/content/docs/reference/troubleshooting.md Three new user-visible error patterns (belongs to a different clone, cannot verify worktree ownership, cannot adopt) are undocumented. Add a "Worktree Belongs to a Different Clone" section with resolution steps.
CLAUDE.md No change needed — @archon/git package description still accurate.

Strengths

  • canonicalRepoPath computation correctly hoisted once outside all three paths
  • DB-preservation invariant correctly implemented (paths 1, 2) and DB non-creation correctly implemented (path 3)
  • Symmetric cross-clone/same-clone test coverage for each guarded path
  • All three try-catches preserve and re-throw the original error — no swallowing
  • isKnownIsolationError/classifyIsolationError cover every new error message — users get actionable guidance
  • Moving verifyWorktreeOwnership to @archon/git removes a cross-package duplication and is the right home

Recommended Actions

  1. Fix log event name (trivial, issue 2)
  2. Decide intent on findLinkedIssueEnv throw-on-first (issue 1) — either continue or add comment
  3. Add structured fields to rejection logs (issue 3)
  4. Preserve err.code via { cause: err } or property assignment (issue 4)
  5. Consider extracting assertWorktreeOwnership helper (nice-to-have, -20 LOC)
  6. Add CHANGELOG.md entry and troubleshooting.md section
  7. Follow-up issue for the findWorktreeByBranch secondary-path gap in providers/worktree.ts:509-521 and for verifyWorktreeOwnership unit tests

Addresses the multi-agent review on #1206:

Code fixes:
- worktree.adoption_refused_cross_checkout log event renamed to match
  CLAUDE.md {domain}.{action}_{state} convention
- verifyWorktreeOwnership now preserves err.code and err via { cause }
  when wrapping fs errors, so classifyIsolationError is robust to Node
  message format changes
- Structured fields (codebaseId, canonicalRepoPath) added to all
  cross-clone rejection logs for incident debugging
- Wrap getCanonicalRepoPath at top of resolve() with classified error
  instead of letting it propagate as an unclassified crash
- Extract assertWorktreeOwnership helper on IsolationResolver —
  centralizes warn-then-rethrow contract, removes duplication
- Dedupe toWorktreePath(existing.working_path) calls in resolver paths
- Add code comment on findLinkedIssueEnv explaining why throw-on-first
  is intentional (user decision — surfaces anomaly instead of masking)

Secondary gap closed:
- WorktreeProvider.findExisting PR-branch adoption path
  (findWorktreeByBranch) now also verifies ownership — same class of
  bug as the main path, just via a different lookup

Tests:
- 8 new unit tests for verifyWorktreeOwnership in @archon/git
  (matching pointer, different clone, EISDIR/ENOENT errno preservation,
  submodule pointer, corrupted .git, trailing-slash normalization,
  cause chain)
- tryBranchAdoption cross-clone test now asserts store.create was
  never called (symmetry with paths 1+2 asserting updateStatus)
- New test for cross-clone rejection in the PR-branch-adoption
  secondary path in worktree.test.ts

Docs:
- CHANGELOG.md Unreleased entry for the cross-clone fix series
- troubleshooting.md "Worktree Belongs to a Different Clone" section
  documenting all four new error patterns with resolution steps and
  pointer to #1192 for the architectural fix
@Wirasm
Copy link
Copy Markdown
Collaborator Author

Wirasm commented Apr 14, 2026

Review Response

Addressed all findings from the multi-agent review. New commit: `c8f2e0c`.

Important Issues — fixed

# Fix
1 `findLinkedIssueEnv` throw-on-first — added a code comment explaining why (intentional: surfaces anomaly instead of masking). User decision documented for future maintainers.
2 Log event renamed `worktree_adoption_refused_cross_checkout` → `worktree.adoption_refused_cross_checkout` (CLAUDE.md `{domain}.{action}_{state}` convention).
3 Added `codebaseId` / `canonicalRepoPath` as structured fields to all cross-clone rejection logs.
4 `verifyWorktreeOwnership` now preserves `err.code` and `err` via `{ cause: err }` when wrapping fs errors. `classifyIsolationError` remains robust to Node message format changes.

Suggestions — addressed

  • Extracted `assertWorktreeOwnership` helper on `IsolationResolver` (~20 LOC removed, Rule of Three satisfied)
  • Deduped `toWorktreePath(existing.working_path)` calls into local `worktreePath` constant
  • Wrapped `getCanonicalRepoPath` at top of `resolve()` with classified error and structured log — no more unclassified crash propagation
  • `tryBranchAdoption` cross-clone test now asserts `store.create` was never called (symmetry with paths 1+2 asserting `updateStatus`)

Unit tests for `verifyWorktreeOwnership` — added

8 new tests in `packages/git/src/git.test.ts` covering the actual `.git` file parsing:

  • Matching pointer → resolves
  • Different clone pointer → throws "belongs to a different clone"
  • Trailing-slash path normalization → resolves
  • `.git` is a directory (EISDIR) → throws + `err.code === 'EISDIR'` preserved
  • `.git` file missing (ENOENT) → throws + `err.code === 'ENOENT'` preserved
  • Submodule pointer (`.git/modules/...`) → throws "not a git-worktree reference"
  • Corrupted `.git` content → throws "not a git-worktree reference"
  • `err.cause` chain preserved for debugging

Secondary gap — fixed in this PR (not follow-up)

The `findWorktreeByBranch` PR-branch-adoption path inside `WorktreeProvider.findExisting` also lacked ownership verification. Same bug class, different lookup. Added the same guard + a new test in `worktree.test.ts`. No follow-up issue needed.

Documentation — added

Validation

  • Type-check: pass
  • Lint: 0 warnings
  • Tests: 304 pass in git + isolation test files (8 new in git, 2 new in isolation)
  • Full test suite: all green

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
packages/isolation/src/resolver.ts (1)

248-260: Log the full ownership error, not just err.message.

verifyWorktreeOwnership() now preserves code and cause, but this helper drops that signal by flattening the error to a string. Logging the error object directly, or at least adding code, would make the new cross-checkout warnings much more useful for incident triage.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/isolation/src/resolver.ts` around lines 248 - 260, The catch in
assertWorktreeOwnership is currently logging only err.message which drops
important fields like code and cause from verifyWorktreeOwnership; update the
getLog().warn call in assertWorktreeOwnership to log the full error object (or
at least include (err as any).code and (err as any).cause) instead of only
err.message so cross-checkout warnings retain the original error metadata from
verifyWorktreeOwnership.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/isolation/src/resolver.ts`:
- Around line 109-131: The canonical-path error is currently rethrown from the
getCanonicalRepoPath block which bypasses resolve()'s existing classification in
createNewEnvironment(); instead of throwing a raw Error, convert this failure
into the resolver's classified result (map to the same "blocked" outcome
createNewEnvironment uses). Replace the throw with a return of the appropriate
IsolationResult (or construct and throw the resolver's domain-specific error
type) carrying status "blocked" (or the creation_failed payload if more
appropriate), include codebase.id, defaultCwd and the original err as cause, and
ensure this path is handled by resolve() the same way createNewEnvironment()
failures are handled so permission/malformed-repo errors become actionable
isolation results; key symbols: getCanonicalRepoPath, canonicalPath, resolve(),
createNewEnvironment(), and the creation_failed/blocked classification.

---

Nitpick comments:
In `@packages/isolation/src/resolver.ts`:
- Around line 248-260: The catch in assertWorktreeOwnership is currently logging
only err.message which drops important fields like code and cause from
verifyWorktreeOwnership; update the getLog().warn call in
assertWorktreeOwnership to log the full error object (or at least include (err
as any).code and (err as any).cause) instead of only err.message so
cross-checkout warnings retain the original error metadata from
verifyWorktreeOwnership.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 0ddf2511-63f2-466c-b890-008c530dcee7

📥 Commits

Reviewing files that changed from the base of the PR and between 49297d0 and c8f2e0c.

📒 Files selected for processing (8)
  • CHANGELOG.md
  • packages/docs-web/src/content/docs/reference/troubleshooting.md
  • packages/git/src/git.test.ts
  • packages/git/src/worktree.ts
  • packages/isolation/src/providers/worktree.test.ts
  • packages/isolation/src/providers/worktree.ts
  • packages/isolation/src/resolver.test.ts
  • packages/isolation/src/resolver.ts
✅ Files skipped from review due to trivial changes (2)
  • packages/docs-web/src/content/docs/reference/troubleshooting.md
  • CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (2)
  • packages/isolation/src/providers/worktree.ts
  • packages/git/src/worktree.ts

Comment on lines +109 to +131
// Compute canonical repo path once — paths 3-6 all need it either for
// ownership verification (cross-clone guard) or for worktree creation.
// Wrap failures so they classify as known isolation errors with actionable
// messages instead of propagating as unclassified crashes.
let canonicalPath: RepoPath;
try {
canonicalPath = await getCanonicalRepoPath(codebase.defaultCwd);
} catch (error) {
const err = error as Error;
getLog().error(
{
err,
errorType: err.constructor.name,
codebaseId: codebase.id,
defaultCwd: codebase.defaultCwd,
},
'isolation.canonical_repo_path_resolution_failed'
);
throw new Error(
`Cannot determine canonical repo path for ${codebase.defaultCwd}: ${err.message}`,
{ cause: err }
);
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Canonical-path failures still escape resolve() as thrown exceptions.

This branch rethrows before the resolver reaches its existing creation_failed/blocked handling in createNewEnvironment(), so permission or malformed-repo failures here will still surface as unclassified crashes instead of an actionable isolation result. Please route this path through the same classification flow, or map it to blocked directly.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/isolation/src/resolver.ts` around lines 109 - 131, The
canonical-path error is currently rethrown from the getCanonicalRepoPath block
which bypasses resolve()'s existing classification in createNewEnvironment();
instead of throwing a raw Error, convert this failure into the resolver's
classified result (map to the same "blocked" outcome createNewEnvironment uses).
Replace the throw with a return of the appropriate IsolationResult (or construct
and throw the resolver's domain-specific error type) carrying status "blocked"
(or the creation_failed payload if more appropriate), include codebase.id,
defaultCwd and the original err as cause, and ensure this path is handled by
resolve() the same way createNewEnvironment() failures are handled so
permission/malformed-repo errors become actionable isolation results; key
symbols: getCanonicalRepoPath, canonicalPath, resolve(), createNewEnvironment(),
and the creation_failed/blocked classification.

verifyWorktreeOwnership previously called path.resolve() on the gitdir
path before embedding it in the error message. On Windows, resolve()
prepends a drive letter to a POSIX-style path (e.g., /other/clone →
C:\other\clone), which:

1. Misled users by showing a path that doesn't match what's actually
   in their .git file
2. Broke a Windows-only test asserting the error contains the literal
   /other/clone path

Compare on resolved paths (correct — normalizes trailing slashes and
relative components for the equality check) but display the raw match
in the error message (recognizable to the user).
@Wirasm Wirasm merged commit fd3f043 into dev Apr 14, 2026
4 checks passed
@Wirasm Wirasm deleted the fix/resolver-worktree-ownership-guard branch April 14, 2026 09:10
Wirasm added a commit that referenced this pull request Apr 14, 2026
Follow-up to #1206 review: the early getCanonicalRepoPath() wrap in
resolve() threw directly, escaping the classification flow that
createNewEnvironment uses. Permission errors, malformed worktree
pointers, ENOENT, etc. surfaced as unclassified crashes instead of
becoming an actionable `blocked` result.

Mirror createNewEnvironment's contract:
- isKnownIsolationError → return { status: 'blocked', reason:
  'creation_failed', userMessage: classifyIsolationError(err) + suffix }
- unknown errors → throw (programming bugs stay visible as crashes,
  not silent isolation failures)

Adds two tests in resolver.test.ts:
- EACCES classifies to "Permission denied" blocked message
- Unknown error propagates as throw

Addresses CodeRabbit review comment on #1206.
Wirasm added a commit that referenced this pull request Apr 14, 2026
)

Follow-up to #1206 review: the early getCanonicalRepoPath() wrap in
resolve() threw directly, escaping the classification flow that
createNewEnvironment uses. Permission errors, malformed worktree
pointers, ENOENT, etc. surfaced as unclassified crashes instead of
becoming an actionable `blocked` result.

Mirror createNewEnvironment's contract:
- isKnownIsolationError → return { status: 'blocked', reason:
  'creation_failed', userMessage: classifyIsolationError(err) + suffix }
- unknown errors → throw (programming bugs stay visible as crashes,
  not silent isolation failures)

Adds two tests in resolver.test.ts:
- EACCES classifies to "Permission denied" blocked message
- Unknown error propagates as throw

Addresses CodeRabbit review comment on #1206.
@coderabbitai coderabbitai Bot mentioned this pull request Apr 14, 2026
Wirasm pushed a commit that referenced this pull request Apr 15, 2026
Worktrees created via `git worktree add` do not initialize submodules — monorepo workflows that need submodule content find empty directories. Auto-detect `.gitmodules` and run `git submodule update --init --recursive` after worktree creation; classify failures through the isolation error pipeline.

Behavior:
- `.gitmodules` absent → skip silently (zero-cost probe, no effect on non-submodule repos)
- `.gitmodules` present → run submodule init by default (opt out via `worktree.initSubmodules: false`)
- submodule init or `.gitmodules` read failure → throw with classified error including opt-out guidance
- Only `ENOENT` on `.gitmodules` is treated as "no submodules"; other access errors (EACCES, EIO) surface as failures to prevent silent empty-dir worktrees

Changes:
- `packages/isolation/src/providers/worktree.ts` — `initSubmodules()` method + call site in `createWorktree()`
- `packages/isolation/src/errors.ts` — collapsed `errorPatterns` + `knownPatterns` into single `ERROR_PATTERNS` source of truth with `known: boolean` per entry; added submodule pattern with opt-out guidance
- `packages/isolation/src/types.ts` + `packages/core/src/config/config-types.ts` — new `initSubmodules?: boolean` config option
- `packages/docs-web/src/content/docs/reference/configuration.md` — documented the new option and submodule behavior
- Tests: default-on, explicit opt-in, explicit opt-out, skip-when-absent, fail-fast on EACCES, fail-fast on git failure, fail-fast on timeout

Credit to @halindrome for the original implementation and root-cause mapping across #1183, #1187, #1188, #1192.

Follow-up: #1192 (codebase identity rearchitect) would retire the cross-clone guard code in `resolver.ts` and `worktree.ts` that #1198, #1206 added. Separate PR.

Closes #1187
@Wirasm Wirasm mentioned this pull request Apr 22, 2026
joaobmonteiro pushed a commit to joaobmonteiro/Archon that referenced this pull request Apr 26, 2026
…am00#1206)

* fix: extend worktree ownership guard to resolver adoption paths (coleam00#1183, coleam00#1188)

PR coleam00#1198 guarded WorktreeProvider.findExisting(), but IsolationResolver
has three earlier adoption paths that bypass the provider layer:

- findReusable (DB lookup by workflow identity)
- findLinkedIssueEnv (cross-reference via linked issues)
- tryBranchAdoption (PR branch discovery)

Two clones of the same remote share codebase_id (identity is derived
from owner/repo). Without these guards, clone B silently adopts clone
A's worktree via any of the three paths.

Changes:
- Extract verifyWorktreeOwnership from WorktreeProvider (private) to
  @archon/git/src/worktree.ts as an exported function, sitting next to
  getCanonicalRepoPath which parses the same .git file format
- Call the shared function from all three resolver paths; throw on
  cross-clone mismatch (DB rows are preserved — they legitimately
  belong to the other clone)
- Compute canonicalRepoPath once at the top of resolve()
- Six new tests in resolver.test.ts covering each guarded path's
  cross-checkout and same-clone behaviors

Fixes coleam00#1183
Fixes coleam00#1188 (part 1 — cross-checkout; part 2 parallel collision deferred
to follow-up alongside coleam00#1036)

* fix: address PR review — polish, observability, secondary gap, docs

Addresses the multi-agent review on coleam00#1206:

Code fixes:
- worktree.adoption_refused_cross_checkout log event renamed to match
  CLAUDE.md {domain}.{action}_{state} convention
- verifyWorktreeOwnership now preserves err.code and err via { cause }
  when wrapping fs errors, so classifyIsolationError is robust to Node
  message format changes
- Structured fields (codebaseId, canonicalRepoPath) added to all
  cross-clone rejection logs for incident debugging
- Wrap getCanonicalRepoPath at top of resolve() with classified error
  instead of letting it propagate as an unclassified crash
- Extract assertWorktreeOwnership helper on IsolationResolver —
  centralizes warn-then-rethrow contract, removes duplication
- Dedupe toWorktreePath(existing.working_path) calls in resolver paths
- Add code comment on findLinkedIssueEnv explaining why throw-on-first
  is intentional (user decision — surfaces anomaly instead of masking)

Secondary gap closed:
- WorktreeProvider.findExisting PR-branch adoption path
  (findWorktreeByBranch) now also verifies ownership — same class of
  bug as the main path, just via a different lookup

Tests:
- 8 new unit tests for verifyWorktreeOwnership in @archon/git
  (matching pointer, different clone, EISDIR/ENOENT errno preservation,
  submodule pointer, corrupted .git, trailing-slash normalization,
  cause chain)
- tryBranchAdoption cross-clone test now asserts store.create was
  never called (symmetry with paths 1+2 asserting updateStatus)
- New test for cross-clone rejection in the PR-branch-adoption
  secondary path in worktree.test.ts

Docs:
- CHANGELOG.md Unreleased entry for the cross-clone fix series
- troubleshooting.md "Worktree Belongs to a Different Clone" section
  documenting all four new error patterns with resolution steps and
  pointer to coleam00#1192 for the architectural fix

* fix(git): use raw .git pointer in cross-clone error message

verifyWorktreeOwnership previously called path.resolve() on the gitdir
path before embedding it in the error message. On Windows, resolve()
prepends a drive letter to a POSIX-style path (e.g., /other/clone →
C:\other\clone), which:

1. Misled users by showing a path that doesn't match what's actually
   in their .git file
2. Broke a Windows-only test asserting the error contains the literal
   /other/clone path

Compare on resolved paths (correct — normalizes trailing slashes and
relative components for the equality check) but display the raw match
in the error message (recognizable to the user).
joaobmonteiro pushed a commit to joaobmonteiro/Archon that referenced this pull request Apr 26, 2026
…leam00#1211)

Follow-up to coleam00#1206 review: the early getCanonicalRepoPath() wrap in
resolve() threw directly, escaping the classification flow that
createNewEnvironment uses. Permission errors, malformed worktree
pointers, ENOENT, etc. surfaced as unclassified crashes instead of
becoming an actionable `blocked` result.

Mirror createNewEnvironment's contract:
- isKnownIsolationError → return { status: 'blocked', reason:
  'creation_failed', userMessage: classifyIsolationError(err) + suffix }
- unknown errors → throw (programming bugs stay visible as crashes,
  not silent isolation failures)

Adds two tests in resolver.test.ts:
- EACCES classifies to "Permission denied" blocked message
- Unknown error propagates as throw

Addresses CodeRabbit review comment on coleam00#1206.
joaobmonteiro pushed a commit to joaobmonteiro/Archon that referenced this pull request Apr 26, 2026
Worktrees created via `git worktree add` do not initialize submodules — monorepo workflows that need submodule content find empty directories. Auto-detect `.gitmodules` and run `git submodule update --init --recursive` after worktree creation; classify failures through the isolation error pipeline.

Behavior:
- `.gitmodules` absent → skip silently (zero-cost probe, no effect on non-submodule repos)
- `.gitmodules` present → run submodule init by default (opt out via `worktree.initSubmodules: false`)
- submodule init or `.gitmodules` read failure → throw with classified error including opt-out guidance
- Only `ENOENT` on `.gitmodules` is treated as "no submodules"; other access errors (EACCES, EIO) surface as failures to prevent silent empty-dir worktrees

Changes:
- `packages/isolation/src/providers/worktree.ts` — `initSubmodules()` method + call site in `createWorktree()`
- `packages/isolation/src/errors.ts` — collapsed `errorPatterns` + `knownPatterns` into single `ERROR_PATTERNS` source of truth with `known: boolean` per entry; added submodule pattern with opt-out guidance
- `packages/isolation/src/types.ts` + `packages/core/src/config/config-types.ts` — new `initSubmodules?: boolean` config option
- `packages/docs-web/src/content/docs/reference/configuration.md` — documented the new option and submodule behavior
- Tests: default-on, explicit opt-in, explicit opt-out, skip-when-absent, fail-fast on EACCES, fail-fast on git failure, fail-fast on timeout

Credit to @halindrome for the original implementation and root-cause mapping across coleam00#1183, coleam00#1187, coleam00#1188, coleam00#1192.

Follow-up: coleam00#1192 (codebase identity rearchitect) would retire the cross-clone guard code in `resolver.ts` and `worktree.ts` that coleam00#1198, coleam00#1206 added. Separate PR.

Closes coleam00#1187
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant